feat: add file upload button with progress bar and improve terminal session limits#753
feat: add file upload button with progress bar and improve terminal session limits#753yuezanhao wants to merge 3 commits into
Conversation
…ession limits - Add upload button to file tree header with file picker dialog - Replace fetch-based upload with XMLHttpRequest for real-time progress tracking - Add progress bar UI in file tree during uploads - Increase file upload size limit from 50MB to 200MB - Extend PTY session timeout from 30min to 4 hours - Increase terminal buffer limit from 5000 to 100000 entries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughexpress.json and express.urlencoded limits were lowered to 10MB; multipart per-file upload limit increased to 200MB (error message updated). PTY session timeout extended to 1 hour, PTY buffering now caps retained chunks and replays buffered output in non-blocking 500-chunk batches. Client adds an XMLHttpRequest upload helper, hook-managed upload with progress, and header/tree UI for selecting and showing upload progress. ChangesServer Upload Capacity
Terminal Session Management
Client-side File Upload with Progress
Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/modules/websocket/services/shell-websocket.service.ts (1)
292-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Array.shift()on a 100,000-element array is O(n) — major hot-path performance regression.
onDatafires for every chunk written by the PTY (potentially thousands of times per second during high-throughput output). Once the buffer is full, every chunk causes a full O(n) re-index of a 100,000-element array. At the previous cap of 5,000 this was tolerable; at 100,000 it will cause measurable CPU spikes and event-loop stalls during sustained terminal output (e.g.,caton a large file,npm install, build output).Consider one of:
- Batch-trim — trim in bulk (O(n) but runs rarely):
⚡ Proposed fix: batch-trim approach
- if (session.buffer.length < 100000) { - session.buffer.push(chunk); - } else { - session.buffer.shift(); - session.buffer.push(chunk); - } + session.buffer.push(chunk); + if (session.buffer.length > 100000) { + // Trim in bulk to amortize the O(n) slice cost + session.buffer = session.buffer.slice(session.buffer.length - 90000); + }
Ring-buffer — O(1) writes; requires adding a
bufferHead: numberfield toPtySessionEntryand reading it out in order on reconnect.Reduce the cap — 100,000 chunks is unlikely to be useful on reconnect (see memory note below) and 10,000–20,000 is typically sufficient for terminal scroll-back.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/modules/websocket/services/shell-websocket.service.ts` around lines 292 - 297, The current onData handler uses session.buffer.shift() which becomes O(n) at the 100,000 cap and will stall the event loop; change to an O(1) ring-buffer by adding a bufferHead: number to the PtySessionEntry, allocate session.buffer as a fixed-capacity array, and on each write store the chunk at index (bufferHead + session.length) % capacity (increment length up to capacity), and when full advance bufferHead = (bufferHead + 1) % capacity instead of calling shift(); update any logic that reads the buffer on reconnect to iterate starting at bufferHead for session.length entries in order.server/index.js (1)
127-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep global JSON/urlencoded limits low; 200MB here widens DoS risk.
Line 127 and Line 138 now allow very large in-memory request bodies for all JSON/form routes. Multipart upload already bypasses this parser and is handled by multer, so this broad increase adds risk without helping the upload path.
Suggested adjustment
app.use(express.json({ - limit: '200mb', + limit: '10mb', type: (req) => { const contentType = req.headers['content-type'] || ''; if (contentType.includes('multipart/form-data')) { return false; } return contentType.includes('json'); } })); -app.use(express.urlencoded({ limit: '200mb', extended: true })); +app.use(express.urlencoded({ limit: '10mb', extended: true }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/index.js` around lines 127 - 138, The global body-parser usage (app.use(express.json(...)) and app.use(express.urlencoded(...))) sets an excessive 200mb limit; reduce the default global limits to a small safe value (e.g. 1mb or another org-approved small limit) and remove the broad 200mb setting, then apply higher limits only on specific routes or handlers that truly need large JSON/urlencoded bodies (or continue using multer for multipart uploads); update the express.json and express.urlencoded configurations in server/index.js accordingly and ensure the multipart skip logic for express.json (contentType.includes('multipart/form-data')) remains intact.
🧹 Nitpick comments (1)
server/modules/websocket/services/shell-websocket.service.ts (1)
33-33: Combined effect of 240-minute timeout and 100,000-chunk buffer creates a large per-session memory footprint.Each idle session holds up to ~100,000 string chunks in memory for up to 4 hours. At a conservative average of 100 bytes per chunk, that is ~10 MB per session. With a handful of concurrent users, abandoned sessions can accumulate tens to hundreds of MB before the timeout fires.
Additionally, on reconnect (lines 221–229) all buffered chunks are replayed synchronously in a
forEach. Sending 100,000 WebSocket frames in a tight loop without back-pressure can flood the client and block the Node.js event loop for a noticeable period.Consider whether 100,000 chunks (vs. the previous 5,000) is practically necessary for the reconnect UX, or whether a more modest cap (e.g., 10,000–20,000) achieves the same scrollback goal with far lower memory cost per session.
Also applies to: 292-292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/modules/websocket/services/shell-websocket.service.ts` at line 33, The current per-session memory and replay strategy is too heavy: reduce the PTY session idle timeout (symbol PTY_SESSION_TIMEOUT) and cap the buffered chunk count to a much smaller value (e.g., 10,000–20,000 instead of 100,000) wherever the buffer constant is defined/managed, and drop oldest chunks when the cap is exceeded; also change the reconnect replay logic (the code referenced at lines 221–229 that does a synchronous buffer.forEach replay) to stream the buffered chunks in controlled batches with back-pressure (e.g., batch size + setImmediate/nextTick or await a short pause between batches and check ws.readyState) so sending doesn’t block the event loop or flood the client. Ensure the names PTY_SESSION_TIMEOUT and the reconnect replay handler/buffer variable are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/index.js`:
- Around line 884-886: The current upload config sets fileSize: 200 * 1024 *
1024 and files: 20 which allows ~4GB of temporary data in os.tmpdir() per
request and risks disk exhaustion; reduce per-file and per-request limits and
add a total-request-size guard: lower fileSize to a safer value (e.g. 10–50MB)
and files to a reasonable concurrent count (e.g. 5), and implement a check that
sums incoming file sizes (or use a library option like maxTotalFileSize if
available) and immediately reject requests when the aggregate exceeds the
threshold; also consider streaming uploads off tmp (use file write stream
handlers or push directly to object storage) and ensure cleanup on errors.
Reference the fileSize and files options and os.tmpdir() in your changes.
In `@src/components/file-tree/hooks/useFileTreeUpload.ts`:
- Around line 109-163: performUpload can run concurrently and clobber shared
state; add a simple guard (preferably an isUploadingRef via
useRef<boolean>(false) or check an existing operationLoading state) at the top
of performUpload to early-return if an upload is in progress, set the guard true
immediately before starting the upload, and clear it in the finally block
alongside the existing
setOperationLoading(false)/setUploadProgress(0)/setDropTarget(null); ensure the
guard is referenced in performUpload (and included in its dependency list if you
use a ref or memoized callback) so overlapping drag/drop or repeated calls
cannot start a second upload while one is active.
---
Outside diff comments:
In `@server/index.js`:
- Around line 127-138: The global body-parser usage (app.use(express.json(...))
and app.use(express.urlencoded(...))) sets an excessive 200mb limit; reduce the
default global limits to a small safe value (e.g. 1mb or another org-approved
small limit) and remove the broad 200mb setting, then apply higher limits only
on specific routes or handlers that truly need large JSON/urlencoded bodies (or
continue using multer for multipart uploads); update the express.json and
express.urlencoded configurations in server/index.js accordingly and ensure the
multipart skip logic for express.json
(contentType.includes('multipart/form-data')) remains intact.
In `@server/modules/websocket/services/shell-websocket.service.ts`:
- Around line 292-297: The current onData handler uses session.buffer.shift()
which becomes O(n) at the 100,000 cap and will stall the event loop; change to
an O(1) ring-buffer by adding a bufferHead: number to the PtySessionEntry,
allocate session.buffer as a fixed-capacity array, and on each write store the
chunk at index (bufferHead + session.length) % capacity (increment length up to
capacity), and when full advance bufferHead = (bufferHead + 1) % capacity
instead of calling shift(); update any logic that reads the buffer on reconnect
to iterate starting at bufferHead for session.length entries in order.
---
Nitpick comments:
In `@server/modules/websocket/services/shell-websocket.service.ts`:
- Line 33: The current per-session memory and replay strategy is too heavy:
reduce the PTY session idle timeout (symbol PTY_SESSION_TIMEOUT) and cap the
buffered chunk count to a much smaller value (e.g., 10,000–20,000 instead of
100,000) wherever the buffer constant is defined/managed, and drop oldest chunks
when the cap is exceeded; also change the reconnect replay logic (the code
referenced at lines 221–229 that does a synchronous buffer.forEach replay) to
stream the buffered chunks in controlled batches with back-pressure (e.g., batch
size + setImmediate/nextTick or await a short pause between batches and check
ws.readyState) so sending doesn’t block the event loop or flood the client.
Ensure the names PTY_SESSION_TIMEOUT and the reconnect replay handler/buffer
variable are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ff126261-0757-45ce-96e2-35b47c541912
📒 Files selected for processing (5)
server/index.jsserver/modules/websocket/services/shell-websocket.service.tssrc/components/file-tree/hooks/useFileTreeUpload.tssrc/components/file-tree/view/FileTree.tsxsrc/components/file-tree/view/FileTreeHeader.tsx
| fileSize: 200 * 1024 * 1024, // 200MB limit | ||
| files: 20 // Max 20 files at once | ||
| } |
There was a problem hiding this comment.
200MB × 20 files allows multi-GB requests and temp-disk exhaustion.
Line 884 with Line 885 now permits up to ~4GB per request in os.tmpdir(). That is a high outage risk under concurrent uploads.
Suggested adjustment
- limits: {
- fileSize: 200 * 1024 * 1024, // 200MB limit
- files: 20 // Max 20 files at once
- }
+ limits: {
+ fileSize: 200 * 1024 * 1024, // 200MB per file
+ files: 5 // Keep worst-case request size bounded
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fileSize: 200 * 1024 * 1024, // 200MB limit | |
| files: 20 // Max 20 files at once | |
| } | |
| fileSize: 200 * 1024 * 1024, // 200MB per file | |
| files: 5 // Keep worst-case request size bounded | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/index.js` around lines 884 - 886, The current upload config sets
fileSize: 200 * 1024 * 1024 and files: 20 which allows ~4GB of temporary data in
os.tmpdir() per request and risks disk exhaustion; reduce per-file and
per-request limits and add a total-request-size guard: lower fileSize to a safer
value (e.g. 10–50MB) and files to a reasonable concurrent count (e.g. 5), and
implement a check that sums incoming file sizes (or use a library option like
maxTotalFileSize if available) and immediately reject requests when the
aggregate exceeds the threshold; also consider streaming uploads off tmp (use
file write stream handlers or push directly to object storage) and ensure
cleanup on errors. Reference the fileSize and files options and os.tmpdir() in
your changes.
Sync with latest upstream changes.
- Reduce global body-parser limit from 200MB to 10MB (uploads use multer) - Replace O(n) Array.shift() with batch slice() for PTY buffer management - Reduce PTY buffer cap from 100K to 20K and session timeout from 240min to 60min - Add batched reconnect replay with setImmediate back-pressure - Add isUploadingRef guard to prevent concurrent upload race conditions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/modules/websocket/services/shell-websocket.service.ts (1)
297-300: ⚡ Quick winBuffer trimming still does O(n) copies on the hot path.
At Line 299, slicing on every chunk past cap copies ~20k entries repeatedly under sustained output. Consider amortized trimming (e.g., trim only when exceeding
cap + batch) or a ring-buffer/deque approach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/modules/websocket/services/shell-websocket.service.ts` around lines 297 - 300, The current hot path pushes chunks into session.buffer and immediately slices when session.buffer.length > PTY_BUFFER_CAP, causing O(n) copies; change to amortized trimming by only trimming when buffer length exceeds PTY_BUFFER_CAP + PTY_BUFFER_BATCH (introduce PTY_BUFFER_BATCH) and then slice down to PTY_BUFFER_CAP, or replace session.buffer with a ring-buffer/deque implementation used by the same service (ShellWebsocketService) so pushes are O(1) and trimming is O(1); update the push site (where session.buffer.push(chunk) is called) to use the new batching threshold or deque API and add the new constants/implementation referenced by PTY_BUFFER_CAP and PTY_BUFFER_BATCH.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/modules/websocket/services/shell-websocket.service.ts`:
- Around line 223-234: The replay loop reads from existingSession.buffer
directly and because of the await/setImmediate it can grow while iterating,
causing replay to expand and stall; fix by snapshotting the buffer before replay
(e.g. make a local copy or capture its length) and iterate over that immutable
snapshot using REPLAY_BATCH_SIZE and the existing ws checks so new PTY output is
not included in the replay and existingSession.ws can be set promptly.
---
Nitpick comments:
In `@server/modules/websocket/services/shell-websocket.service.ts`:
- Around line 297-300: The current hot path pushes chunks into session.buffer
and immediately slices when session.buffer.length > PTY_BUFFER_CAP, causing O(n)
copies; change to amortized trimming by only trimming when buffer length exceeds
PTY_BUFFER_CAP + PTY_BUFFER_BATCH (introduce PTY_BUFFER_BATCH) and then slice
down to PTY_BUFFER_CAP, or replace session.buffer with a ring-buffer/deque
implementation used by the same service (ShellWebsocketService) so pushes are
O(1) and trimming is O(1); update the push site (where
session.buffer.push(chunk) is called) to use the new batching threshold or deque
API and add the new constants/implementation referenced by PTY_BUFFER_CAP and
PTY_BUFFER_BATCH.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 85edd730-6e0a-4846-bb80-8ca9568bff9d
📒 Files selected for processing (3)
server/index.jsserver/modules/websocket/services/shell-websocket.service.tssrc/components/file-tree/hooks/useFileTreeUpload.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/file-tree/hooks/useFileTreeUpload.ts
| const REPLAY_BATCH_SIZE = 500; | ||
| const bufferedChunks = existingSession.buffer; | ||
| for (let i = 0; i < bufferedChunks.length; i += REPLAY_BATCH_SIZE) { | ||
| if (ws.readyState !== WebSocket.OPEN) break; | ||
| const batch = bufferedChunks.slice(i, i + REPLAY_BATCH_SIZE); | ||
| for (const chunk of batch) { | ||
| ws.send(JSON.stringify({ type: 'output', data: chunk })); | ||
| } | ||
| if (i + REPLAY_BATCH_SIZE < bufferedChunks.length) { | ||
| await new Promise((resolve) => setImmediate(resolve)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Replay loop uses a live-growing buffer and can stall reconnection.
At Line 224, bufferedChunks references existingSession.buffer directly. With await at Line 232, new PTY output can be appended between batches, so the loop condition keeps expanding. In noisy sessions, replay can run excessively long, and existingSession.ws (set at Line 237) is delayed.
Suggested fix
-const bufferedChunks = existingSession.buffer;
+const bufferedChunks = existingSession.buffer.slice(); // snapshot at reconnect timeAlso applies to: 237-237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/modules/websocket/services/shell-websocket.service.ts` around lines
223 - 234, The replay loop reads from existingSession.buffer directly and
because of the await/setImmediate it can grow while iterating, causing replay to
expand and stall; fix by snapshotting the buffer before replay (e.g. make a
local copy or capture its length) and iterate over that immutable snapshot using
REPLAY_BATCH_SIZE and the existing ws checks so new PTY output is not included
in the replay and existingSession.ws can be set promptly.
2 participants
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements